Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WeatherStatus] Support more weather codes, change icons, fix style #48353

Merged
merged 6 commits into from
Sep 25, 2024
Merged

[WeatherStatus] Support more weather codes, change icons, fix style #48353

merged 6 commits into from
Sep 25, 2024

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Sep 25, 2024 •
edited by susnux
Loading

This branch is based on the one of #47702.

  • Use icons provided by met.no at https://github.com/metno/weathericons
  • Support more weather codes (a few snow ones)
  • Fix location icon with dark theme
  • Only use icons from @mdi/svg
  • Fix alignment and color issues
  • Make a difference between when no location has been set and when the weather code is unknown/unsupported
Before After
image image
image image

@julien-nc julien-nc added this to the Nextcloud 30.0.1 milestone Sep 25, 2024
@julien-nc julien-nc requested review from jancborchardt, susnux, a team, artonge and Pytal and removed request for a team September 25, 2024 11:18
@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from bf8943e to 64fae5b Compare September 25, 2024 11:20
susnux
susnux reviewed Sep 25, 2024
View reviewed changes
apps/weather_status/src/App.vue Outdated Show resolved Hide resolved
apps/weather_status/src/App.vue Outdated Show resolved Hide resolved
apps/weather_status/src/App.vue Outdated
min-width: 44px !important;
min-height: 44px !important;
.weather-image {
width: 28px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic value? Why 28px?
Our default icons are 16px (which is ~20px MDI icon size)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind using 20px. It's just less visible.
I just raised the size as much as possible to make it easier to see/identify the icon. These weather icons are slightly more important than classic ones IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but then maybe:

    width: calc(var(--default-clickable-area) - 2 * var(--default-grid-baseline));

apps/weather_status/src/App.vue Outdated Show resolved Hide resolved
susnux
susnux requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

License information is important

@susnux
Copy link
Contributor

susnux commented Sep 25, 2024

License information is important

Thank you for fixing, LicenseRef-freepikLicense is now unused so you need to remove that from /Licenses directory :)

@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch 2 times, most recently from 5f0b0de to b487b8b Compare September 25, 2024 14:50
susnux
susnux approved these changes Sep 25, 2024
View reviewed changes
apps/weather_status/src/App.vue Outdated
min-width: 44px !important;
min-height: 44px !important;
.weather-image {
width: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you prefer bigger icons:

Suggested change
width: 20px;
width: calc(var(--default-clickable-area) - 2 * var(--default-grid-baseline));

@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from b487b8b to 1b7ae47 Compare September 25, 2024 15:08
XxTysweezyxX and others added 4 commits September 25, 2024 17:24
@XxTysweezyxX @julien-nc
Fix #30551: Added snowfall option to weather status
1826ba2 
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
feat(weatherstatus): support all weather codes from api.met.no, use t…
bfa1c7e 
…he icons provided by met.no, cleanup UI

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
fix(weatherstatus): make a difference between 'unknown weather code' …
c96bde7 
…and 'no weather info'

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
fix(weatherstatus): fix location marker icon with dark theme, only us…
966a0a9 
…e icons from @mdi/svg

Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from 1b7ae47 to e73c03c Compare September 25, 2024 15:24
@julien-nc

This comment was marked as outdated.

Sign in to view
@julien-nc julien-nc mentioned this pull request Sep 25, 2024
Closed
5 tasks
@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from e73c03c to efcc3e5 Compare September 25, 2024 15:41
@julien-nc
Copy link
Member Author

/compile

@nextcloud-command
chore(assets): Recompile assets
45a3845 
Signed-off-by: nextcloud-command <[email protected]>
@julien-nc julien-nc merged commit b5da085 into master Sep 25, 2024
115 checks passed
@julien-nc julien-nc deleted the enh/30551/weather-status-support-more-codes branch September 25, 2024 16:25
@provokateurin provokateurin mentioned this pull request Sep 25, 2024
Open
@jancborchardt
Copy link
Member

@julien-nc very nice – just one detail: The previous icons worked a bit nicer with the style of Nextcloud, is there a specific reason they got replaced? If not I think it would be good to keep using the previous ones, if that’s ok with you? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@susnux susnux susnux approved these changes

@artonge artonge artonge approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@Pytal Pytal Awaiting requested review from Pytal Pytal was automatically assigned from nextcloud/server-frontend

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: dashboard
Projects
None yet
Milestone
Nextcloud 30.0.1
Development

Successfully merging this pull request may close these issues.

[weather_status] Snowfall
6 participants
@julien-nc @susnux @jancborchardt @artonge @XxTysweezyxX @nextcloud-command